This check won't work. It's impossible to check if include_role resulted in a change in Ansible. There is no proper way to check idempotency other than looking at the playbook output.

@richm added a boolean response from the module to indicate if any action was taken. Rich, please let me know what changes I should make.

https://docs.ansible.com/ansible/latest/dev_guide/developing_modules_general.html#creating-a-module
The module should return a dict. That dict should have a field called changed which is true if the module changed something or false otherwise.

This check won't work. It's impossible to check if include_role resulted in a change in Ansible. There is no proper way to check idempotency other than looking at the playbook output.

But in this case, when the role calls this module, it uses register: snapshot_cmd, so the test playbook can check. Otherwise, you are correct - there is no way to get at the internal state. Several of the roles register the module output or otherwise use some sort of internal variable to indicate the changed state of the role for the purpose of testing for idempotency.

Do you plan to add python unit tests for this module?

Yes. I'll add some unit tests.

This should be an example of using the module directly, not using the role.
Note that using the module directly is not supported, but module scanning tools such as ansible-test and ansible-doc may throw an error if this documentation is not in the correct format.

Ok. The errors are a bit difficult to understand. I was following the example in the storage role. Currently I'm seeing a lot of errors for the docs: https://github.com/linux-system-roles/snapshot/actions/runs/9502379067/job/26190204498 - it looks like the storage role is using a different "-collection-version 1.78.2" vs the snapshot role is using "--collection-version 1.79.0"

I also tried following the examples in: https://docs.ansible.com/ansible/latest/dev_guide/developing_modules_general.html

But had trouble with errors with that format too.

@richm is there a better way to pass snapshot_lvm_set?

I'd like it to be None if it is not set, but the way I've done it here I'm getting "{}\0" when I read it in the python.

Then if you define snapshot_lvm_set as a list of dict in the module, you can just use

        snapshot_lvm_set: "{{ snapshot_lvm_set | d(omit) }}"

So, on the module side, I will remove the code that expects JSON and use dict()?

When I define it as you suggested, the module gets snapshot_lvm_set set to a string, right? But the string is no longer JSON (the values have single quotes).

So, on the module side, I will remove the code that expects JSON and use dict()?

yes

When I define it as you suggested, the module gets snapshot_lvm_set set to a string, right?

No, the module should get snapshot_lvm_set as a dict.

But the string is no longer JSON (the values have single quotes).

in defaults/main.yml you should have

snapshot_lvm_set: {}

This should be a list of dict - see the storage role for examples e.g. https://github.com/linux-system-roles/storage/blob/main/library/blivet.py#L2131

        snapshot_lvm_set=dict(type="list", elements="dict",
                                                  options=dict(name=dict(type="str"),
                                                                         vg=dict(type="str"),
                                                                         lv=dict(type="str"),
                                                                         percent_space_required=dict(type="int"),
....

Sorry - not a list of dict but just a dict

@richm how should I default this to false when snapshot_lvm_all_vgs is not defined. Currently it defaults to "" and that causes an error when the module expects it to be a boolean.

I don't think you need to do this - I believe the register: snapshot_cmd in the snapshot task creates the global variable snapshot_cmd

ok - fixed.

Is it enough to test with 1 thin pool? There seems to be a problem using multiple thin pools with the storage subsystem in el7 - perhaps something to do with blivet on el7? I get the following error:

Failed to commit changes to disk: Process reported exit code 3:   --virtualsize may not be zero.
  Run `lvcreate --help' for more information.

@vojtechtrefny @japokorn any ideas? The storage role thin pool test uses only 1 volume - https://github.com/linux-system-roles/storage/blob/main/tests/tests_create_thinp_then_remove.yml#L37-L43 - which is why it passes on el7

If you really want to test with multiple volumes, then we'll need to define one list of volumes for el7, and another list for el8 and later.

yes, I'll update the test to only use 1 pool.

I'll test multiple pools manually.

@vojtechtrefny @japokorn it looks like if I add:

thin_pool_size: '3g'

whenever I allocate a volume that uses the thin pool it works as expected on RHEL 7 and Centos 7.

Is it enough to test with 1 thin pool? There seems to be a problem using multiple thin pools with the storage subsystem in el7 - perhaps something to do with blivet on el7?

We have quite an old version of blivet on EL7 so there could be a lot of bugs that were fixed between 7 and 8, but I don't remember anything specific about thin provisioning that could be causing this.

The storage role thin pool test uses only 1 volume - https://github.com/linux-system-roles/storage/blob/main/tests/tests_create_thinp_then_remove.yml#L37-L43 - which is why it passes on el7

Looks like something we should add to our test suite.

it looks like if I add: thin_pool_size: '3g' whenever I allocate a volume that uses the thin pool it works as expected on RHEL 7 and Centos 7.

We have some logic in the storage role to calculate size of logical volumes and thin pools when not specified here so it's possible this is somehow broken with older blivet. Can you please check @japokorn?

I'll try to test this scenario with the storage role and I'll report an upstream issue for us to track this.

Looks like the problem was actually invalid size for the second LV:

          - name: lv2
            thin_pool_name: thin_pool
            size: "15"

size: "15" is interpreted as 15 B and on CentOS 7 this is rounded down to 0 KiB so that's why lvcreate is complaining about --virtualsize may not be zero. With newer blivet on Fedora and CentOS 9/10 we automatically adjust this size to the minimal size which explains why it was failing only on CentOS 7.